Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stricter type definition of Subject #5307

Merged
merged 3 commits into from
Apr 3, 2020
Merged

Conversation

hmil
Copy link
Contributor

@hmil hmil commented Feb 10, 2020

Description:

Make type definition of Subject more strict to avoid common bugs when using Subject.

Related issue (if exists):

This issue been discussed extensively in #2852 and #5066 .

Sensitive points

As discussed in the issues linked above, it is possible to omit the argument to next when the Subject is of type Subject<void>.
When the template type is anything other than void, then an argument for next is required.

The primary case where this can be a problem is when people instantiate Subject<any> and then use subject.next() with no arguments. This will break.

The present change makes the default template type of Subject void instead of any. This means that when you write new Subject(), you get a Subject<void> (instead of Subject<any>) and are therefore able to write subject.next() or subject.next(value) if value is either any or undefined.

Other good candidates for default template type are unknown and any. We can discuss whether void is the best choice here, I am not completely sure yet.

@hmil hmil requested a review from cartant February 10, 2020 16:08
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, but I would like to see a test that uses Subject<void> and calls next().

@hmil
Copy link
Contributor Author

hmil commented Feb 11, 2020

@cartant I added tests and some documentation to explain the breaking change and the motivation for choosing void as a default type.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@benlesh
Copy link
Member

benlesh commented Apr 3, 2020

The only issue I see with this is that it will break usages like subject.next(), users will be forced to pass in undefined, which isn't very ergonomic. I'm not sure I want to merge this one. We should ping the TS team on what the best solution is here.

Nevermind, this is only a breaking change for TS 2.x and lower, which we would be breaking for people in v7 anyhow.

@benlesh benlesh merged commit 3e66e66 into ReactiveX:master Apr 3, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants